I started to review but after looking at it for awhile, why not add these resources into a new sub-module here named log-delivery? the implementation is a bit clunky as its written, plus I believe you can send to n-number of destinations (not just 1 S3, 1 Cloudwatch, 1 Firehose)

Let me know - if not in agreement with that approach, I can finish the review here

I started to review but after looking at it for awhile, why not add these resources into a new sub-module here named log-delivery? the implementation is a bit clunky as its written, plus I believe you can send to n-number of destinations (not just 1 S3, 1 Cloudwatch, 1 Firehose)

Let me know - if not in agreement with that approach, I can finish the review here

Hey @bryantbiggs

As far as I know you can only specify one delivery per service and not n. So in this case you can specify a single CloudWatch Log Group and/or a single S3 bucket and/or a single Firehose stream. I know it, cause I have implemented in my daily job.

I have considered it in here cause there's a bit of couplement, I mean, you need to configure the EventBus to be able to log the logs for those destinations. Please do check here, the provided example :)

Happy to discuss further if there's some things I have been missing so far :)

@bryantbiggs In general, it would be a good idea to create a submodule log-delivery, as you suggest (I hadn't considered it). However, I'm not fond of consuming a service module within another service module (EventBridge).

Additionally, only one instance of each log destination type can be added to EventBridge.

However, I'm not fond of consuming a service module within another service module (EventBridge).

No no, not embedding this new sub-module here. Users would define their eventbridge module configuration and then their log-delivery module configuration.

It just feels like a repeat of what has happened with flow logs where now we have to force users to make changes so that we can consolidate that logic in its own module that is then usable in conjunction with our various modules

• Recent PR feat: Add new sub-module for AWS flow log terraform-aws-vpc#1241
• TGW was going to add feat: Add Flow Log Support terraform-aws-transit-gateway#145

@bryantbiggs Updated according to all of the suggestions, except for moving to a separate submodule (I don't have time for more work on this PR anymore). Please review again.

This PR is included in version 4.2.0 🎉